-
Notifications
You must be signed in to change notification settings - Fork 14
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
# 16 calculate_study_day #20
Conversation
Hi @ShiyuC can you take a look at this design and see if it's okay? If so, I can start to add documentation and unit tests |
@yli110-stat697 Hi Rosemary, thanks for creating this PR. It looks good to me. Feel free to move on with documentation and unit test at your convenience |
Hi @yli110-stat697! Could you create a test within the testthat folder so that we could run the function with your test? Thanks! |
Hi yes I can, do we agree on the design already? |
Hi @yli110-stat697 : Thanks for the tip. Will do next time (sorry for the inconvenience!). |
Hi @galachad my pipeline failed due to pandoc installation error. I belive this has nothing to do with my code. How do I fix this? |
…/sdtm.oak into 16_calculate_study_day@devel
…/sdtm.oak into 16_calculate_study_day@devel
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The MR looks good to me. Just commented on one small typo. Thanks @yli110-stat697 for all of your effort!
Co-authored-by: Shiyu Chen <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me.
close #16